Skip to content

Conversation

@korlaxxalrok
Copy link
Contributor

@korlaxxalrok korlaxxalrok commented Dec 9, 2021

Description

Adds the necessary Ansible bits and scripts to deploy claims_hosp like an indicator while keeping intact its current automation routines.

Changelog

  • Adds additonal directory structure and current automation scripts.
  • Gives Ansible the ability to place a templated secrets.py file into an indicator's root directory.
  • Updates vars and vault with new secrets.

Start from Jingjing

  • All the pipeline required code are moved to delphi_claims_hosp.
  • HospClaims could be dropped. But didn't do that currently just for a backup.
  • Switch from print() to logger
  • Add pulling steps from shell scripts to run.py
  • Pulled files are downloaded and stored in ./retrieve_files. All the raw files will be deleted after a production run
  • refactor secrets into params.json
  • Check the validation results. The type of hrr geo_ids are preferred to be string. However, if I change it to zero-filled strings, it does not match the history which generates more problems. So, switched back.

Fixes

Fixes #1623

- Updates deployment playbooks for prod and staging to handle the possibility of a `secrets.py` file.
- Updates vars and vault files with new variable pointers and encryption.
- Adds a new template file that will be rendered as `secrets.py` in the root of the indicators directory.
- Adds new Python and shell scripts to automate `hosp_claims` indicator.
- Adds a symlink for `secrets.py`.
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this still has a ways to go before it's ready to deploy

Co-authored-by: Katie Mazaitis <krivard@cs.cmu.edu>
@korlaxxalrok
Copy link
Contributor Author

@mariajahja Can you take a quick pass through this, maybe check out those comment-cleanup questions?

sanity_check() {
geo=$1
cd "$AUTO_DIR" || exit
python3 sanity_checks.py "$RECEIVING_DIR" "$geo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script performs very basic sanity checks (e.g. percentages between 0 and 100) and can also produce some plots contrasting the new values to ones in the API (see a few line below). If there is a separate validation script run on other indicators such as claims, it would be better to use that and remove this.

@korlaxxalrok korlaxxalrok marked this pull request as draft May 3, 2022 17:53
@jingjtang
Copy link
Contributor

@korlaxxalrok @krivard Could you help me with resolving the conflicts in ansible/vault.yaml? What should I do with it?

@jingjtang jingjtang requested a review from krivard June 15, 2022 14:29
@krivard
Copy link
Contributor

krivard commented Jun 15, 2022

I'll fix the vault; we had an update to the safegraph secrets while this PR was in flight

@jingjtang jingjtang requested a review from qx-teo June 16, 2022 04:47
Copy link
Contributor

@qx-teo qx-teo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on the validator end!

Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • question on need for force parameter
  • additional information needed in final log statement

@jingjtang jingjtang requested a review from krivard June 21, 2022 13:12
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way closer! Two minor items to resolve.

# Allow pytest classes to have one test.
too-few-public-methods
too-few-public-methods,
broad-except
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jingjtang why is broad-except needed?

@jingjtang
Copy link
Contributor

jingjtang commented Jun 21, 2022

why is broad-except needed?

@krivard It's for this warning:

delphi_claims_hosp/download_claims_ftp_files.py:36:11: W0703: Catching too general exception Exception (broad-except)

There can be multiples types of exception error (IndexError, ValueError, ..) if we run into unexpected names

@jingjtang jingjtang requested a review from krivard June 21, 2022 23:55
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@korlaxxalrok is any preparation needed before we merge this or can I click the big green button at will?

@korlaxxalrok
Copy link
Contributor Author

@krivard Looks reasonable. As best I can tell we won't need any prep, but imo best path is to merge and test 👍

@krivard krivard merged commit 74cb9e1 into main Jun 30, 2022
@krivard krivard deleted the productionize-claims_hosp branch June 30, 2022 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Productionize hospital-admissions

7 participants